Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add Entra ID OAuthe2 token support #29032

Merged

Conversation

asaflevi-ms
Copy link
Member

Add support for AAD token credentials

  • a private preview
  • a public preview
  • GA release

@asaflevi-ms asaflevi-ms requested a review from a team as a code owner May 8, 2024 15:22
@asaflevi-ms asaflevi-ms requested review from vicancy and marclerwick and removed request for a team May 8, 2024 15:22
Copy link

openapi-pipeline-app bot commented May 8, 2024

Next Steps to Merge

✅ All automated merging requirements have been met! To get your PR merged, see aka.ms/azsdk/specreview/merge.

Copy link

openapi-pipeline-app bot commented May 8, 2024

Swagger Validation Report

️️✔️BreakingChange succeeded [Detail] [Expand]
There are no breaking changes.
Compared specs (v0.10.9) new version base version
openapi.json 2024-04-01(2a7ae6b) 2024-04-01(main)
️️✔️Breaking Change(Cross-Version) succeeded [Detail] [Expand]
There are no breaking changes.
️️✔️CredScan succeeded [Detail] [Expand]
There is no credential detected.
️⚠️LintDiff: 0 Warnings warning [Detail]
Compared specs (v2.2.2) new version base version
package-2024-04-01 package-2024-04-01(2a7ae6b) package-2024-04-01(main)

The following errors/warnings exist before current PR submission:

Rule Message
⚠️ OperationId OperationId for put method should contain both 'Create' and 'Update'
Location: HealthInsights/stable/2024-04-01/openapi.json#L141
⚠️ EnumInsteadOfBoolean Booleans properties are not descriptive in all cases and can make them to use, evaluate whether is makes sense to keep the property as boolean or turn it into an enum.
Location: HealthInsights/stable/2024-04-01/openapi.json#L1003
⚠️ EnumInsteadOfBoolean Booleans properties are not descriptive in all cases and can make them to use, evaluate whether is makes sense to keep the property as boolean or turn it into an enum.
Location: HealthInsights/stable/2024-04-01/openapi.json#L1210
⚠️ EnumInsteadOfBoolean Booleans properties are not descriptive in all cases and can make them to use, evaluate whether is makes sense to keep the property as boolean or turn it into an enum.
Location: HealthInsights/stable/2024-04-01/openapi.json#L1331
⚠️ EnumInsteadOfBoolean Booleans properties are not descriptive in all cases and can make them to use, evaluate whether is makes sense to keep the property as boolean or turn it into an enum.
Location: HealthInsights/stable/2024-04-01/openapi.json#L1980
⚠️ EnumInsteadOfBoolean Booleans properties are not descriptive in all cases and can make them to use, evaluate whether is makes sense to keep the property as boolean or turn it into an enum.
Location: HealthInsights/stable/2024-04-01/openapi.json#L2007
⚠️ EnumInsteadOfBoolean Booleans properties are not descriptive in all cases and can make them to use, evaluate whether is makes sense to keep the property as boolean or turn it into an enum.
Location: HealthInsights/stable/2024-04-01/openapi.json#L2043
⚠️ EnumInsteadOfBoolean Booleans properties are not descriptive in all cases and can make them to use, evaluate whether is makes sense to keep the property as boolean or turn it into an enum.
Location: HealthInsights/stable/2024-04-01/openapi.json#L2047
⚠️ EnumInsteadOfBoolean Booleans properties are not descriptive in all cases and can make them to use, evaluate whether is makes sense to keep the property as boolean or turn it into an enum.
Location: HealthInsights/stable/2024-04-01/openapi.json#L2051
⚠️ EnumInsteadOfBoolean Booleans properties are not descriptive in all cases and can make them to use, evaluate whether is makes sense to keep the property as boolean or turn it into an enum.
Location: HealthInsights/stable/2024-04-01/openapi.json#L2055
⚠️ EnumInsteadOfBoolean Booleans properties are not descriptive in all cases and can make them to use, evaluate whether is makes sense to keep the property as boolean or turn it into an enum.
Location: HealthInsights/stable/2024-04-01/openapi.json#L2082
⚠️ EnumInsteadOfBoolean Booleans properties are not descriptive in all cases and can make them to use, evaluate whether is makes sense to keep the property as boolean or turn it into an enum.
Location: HealthInsights/stable/2024-04-01/openapi.json#L2086
⚠️ EnumInsteadOfBoolean Booleans properties are not descriptive in all cases and can make them to use, evaluate whether is makes sense to keep the property as boolean or turn it into an enum.
Location: HealthInsights/stable/2024-04-01/openapi.json#L2090
⚠️ EnumInsteadOfBoolean Booleans properties are not descriptive in all cases and can make them to use, evaluate whether is makes sense to keep the property as boolean or turn it into an enum.
Location: HealthInsights/stable/2024-04-01/openapi.json#L2797
⚠️ EnumInsteadOfBoolean Booleans properties are not descriptive in all cases and can make them to use, evaluate whether is makes sense to keep the property as boolean or turn it into an enum.
Location: HealthInsights/stable/2024-04-01/openapi.json#L2802
️️✔️Avocado succeeded [Detail] [Expand]
Validation passes for Avocado.
️️✔️SwaggerAPIView succeeded [Detail] [Expand]
️️✔️TypeSpecAPIView succeeded [Detail] [Expand]
️️✔️ModelValidation succeeded [Detail] [Expand]
Validation passes for ModelValidation.
️️✔️SemanticValidation succeeded [Detail] [Expand]
Validation passes for SemanticValidation.
️️✔️PoliCheck succeeded [Detail] [Expand]
Validation passed for PoliCheck.
️️✔️SpellCheck succeeded [Detail] [Expand]
Validation passes for SpellCheck.
️️✔️Lint(RPaaS) succeeded [Detail] [Expand]
Validation passes for Lint(RPaaS).
️️✔️PR Summary succeeded [Detail] [Expand]
Validation passes for Summary.
️️✔️Automated merging requirements met succeeded [Detail] [Expand]
Posted by Swagger Pipeline | How to fix these errors?

Copy link

openapi-pipeline-app bot commented May 8, 2024

Swagger Generation Artifacts

️️✔️ApiDocPreview succeeded [Detail] [Expand]
 Please click here to preview with your @microsoft account. 
Posted by Swagger Pipeline | How to fix these errors?

Copy link

openapi-pipeline-app bot commented May 8, 2024

@AzureRestAPISpecReview AzureRestAPISpecReview added data-plane TypeSpec Authored with TypeSpec labels May 8, 2024
}
],
"securityDefinitions": {
"AADToken": {
"type": "oauth2",
"description": "The Azure Active Directory OAuth2 Flow",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Azure Active Directory is now Microsoft Entra ID https://learn.microsoft.com/en-us/entra/fundamentals/how-to-rename-azure-ad

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@vicancy .
Is this what you meant ?

Copy link

PR validation pipeline can not start as the pull request is not merged or mergeable - most likely it has merge conflicts.

@asaflevi-ms asaflevi-ms changed the title Add AAD OAuthe2 token support Add Entra ID OAuthe2 token support May 21, 2024
@asaflevi-ms
Copy link
Member Author

@marclerwick, @vicancy , @catalinaperalta , @weidongxu-microsoft
Can one of you review this PR?
If ok, please approve and merge.

CC: @koen-mertens

….tsp

Co-authored-by: catalinaperalta <catalinaperaltah@hotmail.com>
Copy link

PR validation pipeline can not start as the pull request is not merged or mergeable - most likely it has merge conflicts.

@catalinaperalta
Copy link
Member

/azp run

Copy link

PR validation pipeline can not start as the pull request is not merged or mergeable - most likely it has merge conflicts.

Copy link

Pull request contains merge conflicts.

Copy link
Member

@weidongxu-microsoft weidongxu-microsoft left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are we adding AAD support to an existing api-version (that already serving customer)?

Copy link

PR validation pipeline can not start as the pull request is not merged or mergeable - most likely it has merge conflicts.

@asaflevi-ms
Copy link
Member Author

Are we adding AAD support to an existing api-version (that already serving customer)?

@weidongxu-microsoft
Actually, Cognitive Services support it.
We just need to show this in our spec so we can correctly generate SDKs.

Comment on lines 2131 to 2146
"requestId": {
"type": "object",
"description": "An opaque, globally-unique, server-generated string identifier for the request.",
"properties": {
"response": {
"$ref": "#/definitions/Azure.Core.RequestIdResponseHeader"
}
},
"required": [
"response"
]
}
},
"required": [
"error"
"error",
"requestId"
Copy link
Member

@weidongxu-microsoft weidongxu-microsoft Jun 3, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see where these changes comes from.

There is no update to typespec file about these in this PR? -- maybe caused by the conflict in branch?

Copy link
Member Author

@asaflevi-ms asaflevi-ms Jun 3, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@weidongxu-microsoft
Correct.
Fixed by merge from main.

@weidongxu-microsoft
Copy link
Member

weidongxu-microsoft commented Jun 3, 2024

@weidongxu-microsoft Actually, Cognitive Services support it. We just need to show this in our spec so we can correctly generate SDKs.

OK. For a recent procedure change, you can add BreakingChanges-Approved-Bugfix if these is no change in backend, and the typespec/swagger change is to "fix" API definition (in this case auth definition).

Though you need to resolve the conflict first.

@weidongxu-microsoft weidongxu-microsoft merged commit cd47418 into Azure:main Jun 3, 2024
31 of 32 checks passed
Francisco-Gamino pushed a commit to Francisco-Gamino/azure-rest-api-specs that referenced this pull request Jun 5, 2024
* Add AAD OAuthe2 token support

* EntraIdToken

* Update specification/ai/HealthInsights/HealthInsights.OpenAPI/service.tsp

Co-authored-by: catalinaperalta <catalinaperaltah@hotmail.com>

* remove suppression

* update spec after merge

---------

Co-authored-by: koen-mertens <138520871+koen-mertens@users.noreply.github.com>
Co-authored-by: catalinaperalta <catalinaperaltah@hotmail.com>
@asaflevi-ms asaflevi-ms deleted the asaflevi/feature/support-aadtoken branch January 12, 2025 07:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
data-plane TypeSpec Authored with TypeSpec
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants